Hmm. I think it's fine to leave this for follow-up, but also I expect it will be quite straightforward to do.

View changes since the review

do we push a new entry for every closure that we are lowering?

View changes since the review

Something about this feels "off" to me. I think this push should be part of lower_expr_closure rather than here. For one thing, I don't think that these move_expr_bindings should be in scope when we call lower_expr on line 1210, but for another, the call to move_expr_bindings.last()` implies to me that we always push something on the vector. I know that we always push things on the vector when it matters, but it still feels "odd".

Here is an interesting test case:

let v = "Hello, Ferris".to_string();

let r = || {
    || {
        move(move(v.clone())).len()
    }
};

println!("{v}, {}", r());

I don't exactly know what this will do. It depends on whether we visit the contents of move expressions in that collection visitor. I think what I would like it to do is to follow the desgaring. But for sure, with the code as it is right now, when we walk the outer closure "up-front" and we will fail to detect the move because it is contained in a nested closure.

I wonder about doing this a different way. What if we:

• when we enter a closure body, we push an empty vector for "move expressions". When we enter other bodies (e.g., a top-level function), we push a placeholder like None.
• when we encounter a move expression, we check if that vector is Some or None. None results in an error ("move" outside of closure). For Some, we push the expression (unlowered, I think?) into the vector and then we create a new variable and return it.

When we finish processing a closure (i.e., here) we pop that vector and walk over all the things that were collected, and lower the expressions. This should naturally "unfold" in the recursive case the way I expect.

Would this work? Or is there some complexity about the ordering when variables are created or something?

(Alternatively, we always push a vector, not an option, but then we error if, after a non-closure body, the vector is non-empty, but that seems a bit less robust to me (we have to remember to check everywhere).)

View changes since the review

what I want to happen:

let v = "Hello, Ferris".to_string();

let r = || {
    || {
        move(move(v.clone())).len()
    }
};

println!("{v}, {}", r());

becomes

let v = "Hello, Ferris".to_string();

let r = || {
    || {
        move(move(v.clone())).len()
    }
};

println!("{v}, {}", r());

becomes

let v = "Hello, Ferris".to_string();

let tmp0 = v.clone();
let r = move(tmp0) || {
    let tmp1 = tmp0;
    move(tmp1) || {
        tmp1.len()
    }
};

println!("{v}, {}", r());

under my proposed change, this would be Vec<Option<NodeMap<_>>>